imp(types): refactor Default implementations with concrete names #1099

catconcat · 2024-02-24T10:31:43Z

Closes: #1074
Closes: #552
Closes: #373

Description

PR author checklist:

Added changelog entry, using unclog.
Added tests.
Linked to GitHub issue.
Updated code comments and documentation (e.g., docs/).
Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

Reviewed Files changed in the GitHub PR explorer.
Manually tested (in case integration/unit/mock tests are absent).

codecov · 2024-02-24T10:35:01Z

Codecov Report

Attention: Patch coverage is 83.66013% with 25 lines in your changes are missing coverage. Please review.

Project coverage is 66.57%. Comparing base (d0511b7) to head (46910f3).

Files	Patch %	Lines
ibc-core/ics03-connection/types/src/connection.rs	8.33%	11 Missing ⚠️
...s/ics721-nft-transfer/src/handler/send_transfer.rs	0.00%	5 Missing ⚠️
ibc-apps/ics721-nft-transfer/src/module.rs	0.00%	3 Missing ⚠️
ibc-apps/ics721-nft-transfer/types/src/memo.rs	25.00%	3 Missing ⚠️
ibc-apps/ics721-nft-transfer/types/src/class.rs	83.33%	1 Missing ⚠️
ibc-apps/ics721-nft-transfer/types/src/data.rs	66.66%	1 Missing ⚠️
ibc-testkit/src/testapp/ibc/clients/mock/header.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1099      +/-   ##
==========================================
+ Coverage   66.54%   66.57%   +0.03%     
==========================================
  Files         209      209              
  Lines       20617    20636      +19     
==========================================
+ Hits        13719    13738      +19     
  Misses       6898     6898

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

catconcat · 2024-02-26T10:01:26Z

Hi @rnbguy, I am working on this issue and want to ask if I am heading in the right direction or not. At what extent should I refactor/remove these Default implementation, for instance a quite big struct like ConnectionEnd default implementation, I guess we should keep this Default instead, but want to hear your opinion on this.

rnbguy · 2024-02-26T11:17:52Z

That Default implementation shouldn't exist. Default for client_id and counterparty doesn't make sense. The implementation should use ConnectionEnd::new to initialize a new object.

ibc-apps/ics721-nft-transfer/src/handler/send_transfer.rs

ibc-core/ics24-host/types/src/path.rs

catconcat · 2024-02-26T11:34:00Z

@rnbguy Can you take a look at why the no_std substrate check is falling, I don't have much experience in that. Thanks!

rnbguy · 2024-02-26T13:33:19Z

no_std tests fail because they use the latest nightly compiler that accepted RFC 3373. This should be resolved in displaydoc.

Keep working on your PR. We will take care of this in a separate issue.

rnbguy · 2024-02-29T15:26:05Z

you can now merge main onto your branch for successful CI jobs 🙂

rnbguy

🔥 Thanks so much for taking care of this PR. ✨

I added some suggestions. 🙏

ibc-core/ics03-connection/types/src/version.rs

ibc-core/ics24-host/types/src/path.rs

ibc-apps/ics721-nft-transfer/src/module.rs

ibc-core/ics03-connection/types/src/connection.rs

ibc-testkit/src/fixtures/core/channel/mod.rs

…rs into tuan/refactor-default

Farhad-Shabani

Nice improvements through this PR. Thanks @tropicaldog!
I did request a couple more tweaks. Hope that's not too much of a hassle for you. Thanks!

ibc-core/ics03-connection/types/src/version.rs

ibc-core/ics24-host/types/src/identifiers/client_id.rs

ibc-core/ics24-host/types/src/identifiers/connection_id.rs

ibc-core/ics24-host/types/src/path.rs

ibc-core/ics23-commitment/types/src/commitment.rs

rnbguy

Thanks @tropicaldog ! I gave my last PR review and then we merge 🎉

I would have resolved this by myself, but I don't have permission on your branch.

.changelog/unreleased/improvements/1074-refactor-default-implemetation.md

docs/architecture/adr-005-handlers-redesign.md

ibc-core/ics03-connection/types/src/version.rs

ibc-testkit/tests/core/ics04_channel/recv_packet.rs

ibc-testkit/src/testapp/ibc/core/types.rs

ibc-testkit/src/fixtures/core/connection/mod.rs

ibc-testkit/src/fixtures/core/connection/conn_open_try.rs

ibc-testkit/src/fixtures/core/connection/conn_open_init.rs

…rs into tuan/refactor-default

rnbguy

Great work ! ✨

Thanks for the contribution. 🙏

Farhad-Shabani

🎉

catconcat added 2 commits February 24, 2024 17:30

use concrete channel order enum

4f68431

Merge branch 'main' into tuan/refactor-default

59df516

catconcat changed the title ~~imp(types): Use concrete channel order enum~~ imp(types): refactor Default implementations with concrete names Feb 25, 2024

catconcat added 8 commits February 26, 2024 13:09

refactor(ics04): remove Version::default()

f194383

refactor(ics04): remove TimeoutHeight::default()

de47605

refactor: remove channelId::default()

e384dca

fix: Version::default() to Version::empty()

526acde

remove derive default from Timestamp

f4f0dae

refactor default builder

000959e

remove Sequence::default()

2cb0808

remove Memo::default()

6a31d85

rnbguy reviewed Feb 26, 2024

View reviewed changes

ibc-apps/ics721-nft-transfer/src/handler/send_transfer.rs Outdated Show resolved Hide resolved

rnbguy reviewed Feb 26, 2024

View reviewed changes

ibc-core/ics24-host/types/src/path.rs Outdated Show resolved Hide resolved

Farhad-Shabani mentioned this pull request Feb 28, 2024

check-substrate job fails due to displaydoc issue with the latest Rust nightly compiler #1105

Closed

catconcat added 10 commits February 29, 2024 23:50

refactor: add ChannelId::zero()

bab6785

Merge branch 'main' into tuan/refactor-default

332e9e6

add From<&str> implmentation

e800ea8

refactor(ibccore): remove ClientId::default()

c957bd9

refactor(ibccore): remove version::Default()

36fba96

refactor(ibccore): remove ConnectionEnd::default()

48656b7

refactor(ibc-apps): remove TracePath::default()

f1c1f13

refactor(ibc-core): remove ProofSpecs::default()

436703f

fix(ibc-app): remove upgrade path default

6d41e93

remove trust threshold default

04ae141

catconcat added 3 commits March 9, 2024 02:19

refactor(ibc-testkit): remove Default() of ClientId, ConnectionId,...

8aea66d

chore: add unclog

37ce288

Merge branch 'main' into tuan/refactor-default

3572e3b

catconcat marked this pull request as ready for review March 9, 2024 18:27

rnbguy requested changes Mar 11, 2024

View reviewed changes

catconcat added 4 commits March 12, 2024 00:11

refactor: add Connection::zero()

cd2addd

use into() instead of ::from()

ec84de5

refactor: add Version::compatibles()

7750afd

Merge branch 'tuan/refactor-default' of github.com:notional-labs/ibc-…

f279965

…rs into tuan/refactor-default

Farhad-Shabani suggested changes Mar 12, 2024

View reviewed changes

catconcat added 4 commits March 12, 2024 09:38

refactor(ics24-host): make 07-tendermint-0 a constant in tests

a860979

chore: remove commented out impl Default

a206527

add dummy ClientId

596eccb

refactor(ics24-host): remove From<&str> to avoid panic on host

6aba54f

catconcat force-pushed the tuan/refactor-default branch from 60a9943 to 6aba54f Compare March 12, 2024 04:31

catconcat added 3 commits March 12, 2024 11:37

remove get_compatible_versions

9f4bd71

fix syntax error when import Version

91dedf1

chore: run cargo fmt

c7e86e8

Farhad-Shabani added this to the 0.51.0 milestone Mar 12, 2024

rnbguy requested changes Mar 12, 2024

View reviewed changes

catconcat added 5 commits March 12, 2024 23:25

revert markdown format

4fd5dca

Merge branch 'main' into tuan/refactor-default

cd22fb2

Merge branch 'tuan/refactor-default' of github.com:notional-labs/ibc-…

fabc36c

…rs into tuan/refactor-default

various refactor

fdf2a7c

chore: update unclog

46910f3

rnbguy approved these changes Mar 12, 2024

View reviewed changes

rnbguy enabled auto-merge (squash) March 12, 2024 17:50

Farhad-Shabani approved these changes Mar 12, 2024

View reviewed changes

rnbguy merged commit 04520a8 into informalsystems:main Mar 12, 2024

imp(types): refactor Default implementations with concrete names #1099

imp(types): refactor Default implementations with concrete names #1099

Uh oh!

Conversation

catconcat commented Feb 24, 2024 • edited by rnbguy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

PR author checklist:

Reviewer checklist:

Uh oh!

codecov bot commented Feb 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

catconcat commented Feb 26, 2024

Uh oh!

rnbguy commented Feb 26, 2024

Uh oh!

Uh oh!

Uh oh!

catconcat commented Feb 26, 2024

Uh oh!

rnbguy commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnbguy commented Feb 29, 2024

Uh oh!

rnbguy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Farhad-Shabani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rnbguy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rnbguy left a comment

Choose a reason for hiding this comment

Uh oh!

Farhad-Shabani left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

catconcat commented Feb 24, 2024 •

edited by rnbguy

Loading

codecov bot commented Feb 24, 2024 •

edited

Loading

rnbguy commented Feb 26, 2024 •

edited

Loading